I'll finish the review on Monday.

@raymondfeng what is your scenario where you need to control the order in which the boot scripts are invoked and where the script name is not good enough? The same thing you are trying to achieve in this PR can be achieved to a certain degree by prepending a number to the script name, e.g. boot/200-run-later.js.

One of the reasons why I am against the idea of using numbers for ordering boot scripts, is that external modules (yeoman generators, loopback components) need to specify the ordering in relative means (run this after that) instead of using an absolute position (run this as the script no. 3). This is not a new problem, we have to solve it for middleware ordering too - see #44.

The second reason is more practical: when the order is defined by both file names and optional order properties exported by these files, it is rather difficult to construct a mental image of the complete boot order, as one has to read all boot files, check for order properties that may or may not be there, and also remember what is the default value for that property.

BTW this is the reason why I asked you to split this patch into two PRs. While I did not review the async boot scripts in detail, it seems rather uncontroversial, thus I expect it can be reviewed and merged quickly.

The ordering part of the PR was driven by a use case to discover models programmatically from a database during the boot process and expose the discovered models as REST APIs. The code needs to be executed before the API explorer/REST middleware is loaded.

I'm fine to refine the mechanism of ordering, for example, we can have boot.json or file name patterns.

It's a bit tricky to split the PRs. I don't know how to create a PR on top of another PR as the tests also use the async booting.

Let's keep this PR focused on boot script ordering (perhaps change the title) then. Can you submit Add support for async boot scripts as a new PR please? Once it's landed, you can rebase this PR on top of the new master.

OK, I reverted the changes for ordering and leave this PR for async booting.

I believe that I have addressed your comments with the last push. Please verify. Now with the behavior of async.eachSeries(), we can handle both sync & async scripts. For example, if we have two scripts:

s1.js:

module.exports = function(app) {
...
}

s2.js

module.exports = function(app, callback) {
...
}

If no callback is provided by boot(), s1 and s2 are executed synchronously and the callback is no-op for s2. When callback is provided by boot(), s1 is executed synchronously and s2 is executed asynchronously but they are still in series.

The algorithm looks good now. It is much simpler that the initial version, which is almost always a good sing. I left a bunch of comments on the coding style.

On the second thought, the tests need more changes. At the moment, they check only the fact that the scripts were executed, but they don't verify the order of execution.

Here is a proposed solution. Note how the unit tests nicely describe how the boot process works.

Edit: I fixed the tests to reflect the fact that all scripts are required first and only then the exported functions are called.

// boot/bar.js
process.bootFlags.push('barLoaded');
module.exports = function(app, callback) {
  process.bootFlags.push('barStarted');
  process.nextTick(function() {
    process.bootFlags.push('barFinished');
    callback();
  });
};

// boot/barSync.js
process.bootFlags.push('barSyncLoaded');
module.exports = function(app) {
  process.bootFlags.push('barSyncExecuted');
});

// boot/foo.js
process.bootFlags.push('fooLoaded');

// test - boot with callback
expect(process.bootFlags).to.eql([
  'barLoaded',
  'barSyncLoaded',
  'fooLoaded'
  'barStarted',
  'barFinished',
  'barSyncExecuted',
]);

// test - boot without callback
expect(process.bootFlags).to.eql([
  'barLoaded',
  'barSyncLoaded',
  'fooLoaded'
  'barStarted'
  // everything else happens in the next tick and later
]);

setTimeout(function() {
  // all async boot scripts are done by this time
  expect(process.bootFlags).to.eql([
    'barLoaded',
    'barSyncLoaded',
    'fooLoaded'
    'barStarted',
    'barFinished',
    'barSyncExecuted',
  ]);
}, 10);

Now that the order of execution is nicely visible in the comment above, I see that this patch is introducing a backwards-incompatible change. When boot is called without a callback, the exported functions are executed immediately after the file is loaded. In the proposed implementation, the exported functions are called only after all boot scripts were loaded.

@raymondfeng Is that the reason why you had different paths for boot(app, opts, callback) and boot(app, opts)?

I am wondering what is the likelihood that somebody is doing stuff outside the exported function that depends on changes made by a previous boot script in its exported function. It seems very unlikely to me, thus I think we can get away with making the change.

One more thing @raymondfeng - please add debug statements at least in the place where a script is loaded and when the exported function is either skipped or called. Ideally there will be all three things reported - script is loaded via require, exported function is started, exported function finished.

My vision is that with DEBUG turned on, the user should be able to see an output similar to the expected array in the test outlined by my comment above, so that he can reason about the order in which his code was executed.

DEBUG statements added.

@raymondfeng I am not happy with the way how you are working around the require.cache by adding a new test fixture instead of using appdir. It's just a matter of time until this problem bites us again.

I am not against having sync and async fixture per se, but we should fix the real problem too. Also note that the current tests do not test sync call of boot with async boot scripts, which is IMO an important scenario to cover too.

Here is the solution for require.cache:

The proper solution is very easy:

• add beforeEach(appdir.init) to the top-level describe block

• modify simpleAppInstructions and simpleAsyncAppInstructions to something like this:

  var fs = require('fs-extra'); // this should go to the top of the file
  fs.copySync(SIMPLE_APP, appdir.DIR);
  return boot.compile(appdir.DIR);

Issues addressed.

One more minor comment which you don't necessarily have to address.

The patch LGTM.

If you are going to release a new version, make it a minor, not a patch please. This PR, #54 and #43 added new features.